Hi @joesaunderson!
Do you mind to add a bunch of tests please?

Hi @joesaunderson! Do you mind to add a bunch of tests please?

I can try, there aren't many existing tests though, so not sure where I'll be starting from.

Hi @joesaunderson! Do you mind to add a bunch of tests please?

I can try, there aren't many existing tests though, so not sure where I'll be starting from.

Yep @Vincz this is a bit beyond my knowledge of this bundle, there's nothing to extend and I'm getting into the internals of the system.

Hi @Vincz, @joesaunderson

I don't understand the need for this change.
Currently we're experiencing a changed and unexpected behavior in our application.

I our use-case we're creating an order and filling customer data based on the input.
Customer may choose a different delivery address.

differentDeliveryAddress:
    type: "Boolean!"
deliveryFirstName:
    type: "String"
    validation:
        -   NotBlank:
                message: "Please enter first name of contact person"
                groups: "differentDeliveryAddressWithoutPreselected"

The validation group is then computed based on the value of the differentDeliveryAddress field - so when different delivery address is true (customer wishes to delivery goods to different address), the delivery fields become mandatory (regardless the input data).

But after this change, the deliveryFirstName field is never validated if not passed in the mutation. This leads to the different behavior to data if passed and not, which is error-prone approach.

Can you explain the use-case benefiting from this change? And do you have an idea how to solve the case I described?

The use case benefitting this change. Imagine a mutation like so:

edit_entity($entity)

Where entity is all the editable fields on that entity.

If you only want to edit some properties of the entity, you can pass only the properties you want to edit.

Previously, if you did this, it ran the validation rules for every properly of that entity (or input object in this case) regardless of whether they were passed on not.

But that way you have to count on the data you're passing down the application. So it's really easy to make a mistake and set data that should not pass the validation.

Not sure I fully understand your use case so forgive me...

But isn't it correct that when not passing the deliveryFirstName, it shouldn't be validated?

It should only be validated when it's passed (which is when differentDeliveryAddress is true?).

But that way you have to count on the data you're passing down the application. So it's really easy to make a mistake and set data that should not pass the validation.

Where does the counting element come in to it? This way the data you pass in is validated, the data you don't pass in isn't validated?

If I set the differentDeliveryAddress = true, the I want to validate the deliveryFirstName regardless if passed in mutation.
Because differentDeliveryAddress means deliveryFirstName field has to be set.

Now I have to be sure I pass all field even though the schema do not require them, otherwise invalid data may be set

If I set the differentDeliveryAddress = true, the I want to validate the deliveryFirstName regardless if passed in mutation.

Because differentDeliveryAddress means deliveryFirstName field has to be set.

Now I have to be sure I pass all field even though the schema do not require them, otherwise invalid data may be set

I understand it better now. Devils advocate... if you're essentially requiring that field when differentDeliveryAddress = true, shouldn't your application ensure it's always passed too?

@Vincz suggested this behaviour could be a config setting, so maybe that could be an option?

validationMode: full | partial

Hi @grossmannmartin, @joesaunderson
Yes, in the end both of yours uses cases make sense. So, we should definitely add an option.
What do you think, should be the default behavior?
I think we should be able to set the default behavior in the bundle config as suggested but we should also be able to override this in the input config itself.
Does any of you want to take care of this PR?

I should have some time to look at it Tuesday. Can you point me in the direction of where config is used in other places?

Hi, thanks for understanding and the dialog :)

From my point of view, the original behavior is better as a default, because otherwise the rest of the application has to know what was passed as arguments and it's a behavior that already was.
But I am completely ok with the opposite as default as long as it's configurable :)

If necessary, I can make some time at the end of the week... sooner is not possible for me, sorry

@Vincz @grossmannmartin I had a brief go at this today. It adds a new config option validation_mode to the default config.

I tried to add it to the field level too (under validation.mode) but got lost in the internals of this repository that I don't understand. Feel free to extend it, or merge it as is (as part 1).

I believe it puts us in a good place for full / partial validation.

#1185

Hi @Vincz @joesaunderson

I discovered that this modification causes 2 validation issues in certain scenarios:

1. Invalid data to bypass validation rules
2. Internal server Error for valid data

Prerequisites:
schema:

CreatePageElementInput:
    type: input-object
    config:
        fields:
            translations:
                type: '[PageElementTranslationInput!]'
                validation: cascade

PageElementTranslationInput:
    type: input-object
    config:
        fields:
            placeholder:
                type: String!
            tooltip:
                type: String
                validation:
                    -   Symfony\Component\Validator\Constraints\Length:
                            max: 5
            language:
                type: String!

valid input data related to schema (no issues):

{
  translations: [
    {
      placeholder: "enter",
      language: "english",
      tooltip: "text"
    },
    {
      placeholder: "введіть",
      language: "ukrainian",
      tooltip: "текст"
    }
  ]
}

Scenario 1) Invalid data to bypass validation rules

{
  translations: [
    {
      placeholder: "enter",
      language: "english",
      tooltip: "invalid text with length > 5"
    },
    {
      placeholder: "введіть",
      language: "ukrainian"
    }
  ]
}

Reason:
While processing second object (without "tooltip"), $metadata will not contain validation for "tooltip" field.
Taking into account addMetadata implementation $this->metadataPool[$metadata->getClassName()] = $metadata;, this call $this->metadataFactory->addMetadata($metadata); will replace validation rules.

Scenario 2) Internal server Error for valid data

{
  translations: [
    {
      placeholder: "enter",
      language: "english"
    },
    {
      placeholder: "введіть",
      language: "ukrainian",
      tooltip: "норм"
    }
  ]
}

Reason:
While processing first object (with "tooltip"), $metadata will contain validation for "tooltip" field.
And, it will try to validate first object. Taking into account that this code will not be reached due to continue introduced in this PR.

} else {
    $rootObject->$property = $inputData[$property] ?? null;
}

It gives us error: Property Overblog\GraphQLBundle\Validator\ValidationNode::$tooltip does not exist

@ITernovtsii see #1185

I allowed the behaviour to be configurable, but I haven't had time to write tests / take it forward.

Feel free to take it on if this is pressing

@joesaunderson I think this change should be removed from the released version first, as "partial" mode will not work properly anyway,
There is no reason to build the switch between 2 modes while only one mode works properly.
I reviewed the core a bit and don't see any simple solution for how this can be implemented without the validation break.
It looks like we may need metadata for each object, which will slow down request processing, and require significant changes in the code.

@joesaunderson I think this change should be removed from the released version first, as "partial" mode will not work properly anyway,

There is no reason to build the switch between 2 modes while only one mode works properly.

I reviewed the core a bit and don't see any simple solution for how this can be implemented without the validation break.

It looks like we may need metadata for each object, which will slow down request processing, and require significant changes in the code.

Partial mode does work, and is working for our use case in a production app for millions of users 😀

I think both your scenarios are when an input object is passed as an array, so maybe we need to adjust the check for that (does the field exist on any object in the array).

Sorry, but I disagree that it makes sense to keep it because it works in some cases. Especially taking into account that bypass validation can be considered as a security issue.
Potentially, it can be easily solved on application level by ignoring null fields.

Issue is reproducible not only with arrays, but with any schema where same input type used twice in the request.

For example, if you have address input type, and passing it as person's deliveryAddress plus as homeAddress. User who knows about this issue will be able to bypass validation rules, and set invalid county code to delivery address.
And those two types may exist in different depth levels of input data structure. So, it will require to iterate through the whole data tree to identify if there's object of the same type.

And I don't think it will be as simple as adjust this check, as it breaks consistency. Same object will be validated differently based on another object in the request.

There's was also another suggestion from @Vincz so allow the validation mode to be set at the config level (PR above) or at a mutation level.

Maybe you could extend my PR to add that too, allowing it to be set at a mutation by mutation basis?

@joesaunderson
@Vincz
@grossmannmartin

This change should not have been accepted. It's not fixing a bug; it's introducing one.

@joesaunderson in your initial issue, you showed this example:

config:
    description: "An input object for editing a merchant"
    fields:
        name:
            type: "String"
            validation:
                - NotBlank: ~
        description:
            type: "String"

So here you are saying that name should not be a blank string, a blank array, false, or null. This means that name MUST be provided; otherwise, it's null and is invalid. Then you pass this value, where name is missing:

mutation {
  edit(set: {description: "Foo"}) {
    id
  }
}

And the validation fails, which is a perfectly correct behavior. Am I missing something? If you don't want to provide name, why make it mandatory? Because that's precisely what Assert/NotBlank does.

What you should have done instead is this:

config:
    description: "An input object for editing a merchant"
    fields:
        name:
            type: "String"
            validation:
                - NotBlank:
                    allowNull: true # add this option
        description:
            type: "String"

In human language, this means:

1. If name is NOT provided, that's ok, because null is now allowed.
2. If name is provided, then it should not be an empty string, an empty array or false.

This way, you can achieve your goal of not validating fields that are not provided.

I will open a roll-back PR because the current behavior is not correct and is counterintuitive. If we look into your test, we will see this:

Person:
    type: input-object
    config:
        fields:
            firstName:
                type: String
                validation:
                    - NotBlank: ~
                    - NotNull: ~
            surname:
                type: String
                validation:
                    - NotBlank: ~
                    - NotNull: ~

The validation constraints say the fields are required, yet we skip validation. This raises a logical question: why do we even need the NotBlank and NotNull constraints then?

@murtukov I haven't worked on this for quite some time, but the thinking behind this was... if you want to mutate an entity that has say 40 properties

How do you mutate it without having to spread (pass in) every property, every time.

@joesaunderson

How do you mutate it without having to spread (pass in) every property, every time.

If you don't want to pass in every property, don't mark them required, simple as that. You configured your fields as NotBlank, which makes them required. Just remove that constraint.

@joesaunderson

How do you mutate it without having to spread (pass in) every property, every time.

If you don't want to pass in every property, don't mark them required, simple as that. You configured your fields as NotBlank, which makes them required. Just remove that constraint.

I suppose it's how you interpret that rule.

I read that as, if passed, not blank. Not, it has to be passed (required) every time.

Otherwise, you need to fetch the entire object, in order to mutate a single field.

Say you want to update just the name, with a simple form. You'd be over fetching 39 properties for the sake of updating one.

But I can see both sides, hence the previous discussion of a 'mode' that could be configured.

@joesaunderson, my friend, the validator already works the way you want it. By default, it does NOT require you to pass all the properties. Here is an example:

Person:
    type: input-object
    config:
        fields:
            firstName:
                type: String
                validation:
                    - Length: { min: 1 }
            lastName:
                type: String
                validation:
                    - Length: { min: 1 }
            age:
                type: Int
                validation:
                    - Range: { min: 18, max: 120 }
            email:
                type: String
                validation:
                    - Email: ~
            gender:
                type: String
                validation:
                    - Choice: ['male', 'female', 'none']

There are 5 fields, all with validation, and none is required. You can update any field you want, without passing the others:

mutation {
    updatePerson(person:  {
        firstName: "John"
    })
}

This will pass, and the validation will not be triggered on missing fields.

However, if you sent this:

mutation {
    updatePerson(person:  {
        firstName: "John"
        lastName: ""
    })
}

it will fail, because the lastName is passed but is blank.

It is the NotBlank constraint that makes the fields required by disallowing them to be null. But even with NotBlank you can allow the missing fields skip the validation using the option allowNull:

Person:
    type: input-object
    config:
        fields:
            firstName:
                type: String
                validation:
                    - NotBlank: {allowNull: true}
            lastName:
                type: String
                validation:
                    - NotBlank: {allowNull: true}
            age:
                type: Int
                validation:
                    - NotBlank: {allowNull: true}
                    - Range: { min: 18, max: 120 }
            email:
                type: String
                validation:
                    - NotBlank: {allowNull: true}
                    - Email: ~
            gender:
                type: String
                validation:
                    - Choice: ['male', 'female', 'none']

And it will trigger the validation only on the passed properties.

@murtukov

And what if you're not constructing validation like the above, and instead linking to validation of an entity.

Then, it falls apart (as we do want the NotBlank when validating the entity, but we don't want it when mutating it, if not being passed).

Hence the need for this change.

I'd suggest you take my branch forward above to implement two modes.

What do you mean by this?

as we do want the NotBlank when validating the entity, but we don't want it when mutating it

I think you wanted to say

as we do want the NotBlank when creating the entity, but we don't want it when mutating it

If that's the case, you use validation groups for that.

I'd suggest you take my branch forward above to implement two modes.

There is no need for "two modes". You already have all the required instruments to achieve all the mentioned scenarios.